[feature] ASIC-focused multicast replication and dendrite API #14
Conversation
zeeshanlakhani
force-pushed
the
zl/p4-mcast
branch
3 times, most recently
from
4e2fc4b to
1ce4908
Compare
Includes:
* Multicast Group API management: add, modify, delete, reset, SSM handling
* sidecar.p4 tofino_asic updates to integrate multicast packet replication
* Table mangagement for ASIC switch tables for multicast
* integration tests and test utility additions
zeeshanlakhani
force-pushed
the
zl/p4-mcast
branch
from
1ce4908 to
df62508
Compare
zeeshanlakhani
force-pushed
the
zl/p4-mcast
branch
from
2426e18 to
4a945a8
Compare
|
I'll add some notes to the PR tomorrow morning (to explain through pieces). |
| ( 0, false, true, true, _, USER_SPACE_SERVICE_PORT, true, _, _, _, _, _, _ ) : forward_from_userspace; | ||
| ( 0, false, false, _, _, _, false, true, _, _, _, _, _ ) : forward_to_userspace; | ||
| ( 0, false, false, _, true, _, _, _, _, _, _, _, _ ) : forward_to_userspace; | ||
| // Link-local multicast |
There was a problem hiding this comment.
Since we're changing quite a bit around link-local multicast, we need to test this e2e with ddm.
Includes:
* make sure to avoid switch table apply on drop-specific code in sidecar
zeeshanlakhani
force-pushed
the
zl/p4-mcast
branch
from
b09c63c to
c9224db
Compare
zeeshanlakhani
force-pushed
the
zl/p4-mcast
branch
from
578d926 to
21d9274
Compare
Includes: * kyle's patch on scopedguard/group changes (after modifications) with slight mod on del
zeeshanlakhani
force-pushed
the
zl/p4-mcast
branch
from
2c50521 to
6f4082e
Compare
|
@FelixMcFelix should now be in a good spot API-wise. |
dpd/src/mcast/mod.rs
Outdated
| DpdError::Invalid( | ||
| "external group ID should have been pre-allocated".to_string(), | ||
| ) |
There was a problem hiding this comment.
This feels like an invariant maintained server-side, rather than something derived from poor user input. DpdError::Invalid will convert into an HTTP 400, whereas this seems like a 500 on that note. DpdError::Other might fill that niche?
dpd/src/mcast/mod.rs
Outdated
| DpdError::Invalid( | ||
| "underlay group ID should have been pre-allocated".to_string(), | ||
| ) |
There was a problem hiding this comment.
Ditto here on something which can convert to a 500?
|
To determine if we can target Basic TCP:
|
|
Re-targeted to
|
|
Pinging @Nieuwejaar and @rcgoodfellow for any other looks here beyond @FelixMcFelix's extensive review! |
|
Unless @Nieuwejaar has any more feedback I'd say let's get this landed. |
dpd/src/api_server.rs
Outdated
| let group_id = query_id.into_inner().group_id; | ||
|
|
||
| // If a group ID is provided, get the group by ID | ||
| if let Some(group_id) = group_id { |
There was a problem hiding this comment.
Oh, I see. This was a special case so you could get by id, since we already have an endpoint for ip. I missed that there was a similar special case for DELETE. That's pretty messy.
Is there a reason we can't have /multicast/groups/{group_id}? We already have a MulticastGroupIdParam::to_identifier() operation that could convert it from either format.
| @@ -17,6 +17,7 @@ p4_artifacts* | |||
| # Editor config | |||
| .vscode | |||
| .dir-locals.el | |||
| bacon.toml | |||
There was a problem hiding this comment.
Should this be removed?
There was a problem hiding this comment.
I had been using it (and it is a tool, but fine to remove it).
There was a problem hiding this comment.
It's harmless. I just wasn't sure if you meant to leave it in or not. If you want to keep it, feel free.
| const int IPV6_NEIGHBOR_SIZE = 512; // ipv6 neighbor cache | ||
| const int SWITCH_IPV4_ADDRS_SIZE = 512; // ipv4 addrs assigned to our ports | ||
| const int SWITCH_IPV6_ADDRS_SIZE = 512; // ipv6 addrs assigned to our ports | ||
| const int IPV4_MULTICAST_TABLE_SIZE = 1024; // multicast routing table(s) for IPv4 |
There was a problem hiding this comment.
How did you arrive at a size of 1024 for all of the multicast tables? I suspect that when we add customer support for IPv6, we're going to have to bump up the existing IPv6 tables, and we don't have a lot of headroom. Unless we know that we need the multicast tables to be this big, I would be inclined to start with something much smaller and grow them if the customer hits the limit. That will be much lower risk than trying to shrink them after this is in use at a customer site.
There was a problem hiding this comment.
@Nieuwejaar I thought 1024 was starting small, based on what mcast-interested customers had been bringing up (which was larger). I could break up the V6 constants as they're used for different table components, but maybe @rcgoodfellow has thoughts here?
There was a problem hiding this comment.
If this is the size we need, then I guess it's the size we need.
I suspect we're going to need to start packaging multiple sidecar binaries, optimized for different customer sites. We're just not going to be able to support a large number of IPv4 routes, a large number of IPv6 routes, and large scale multicast all at the same time.
There was a problem hiding this comment.
Yeah, I was a bit worried this would be the direction we have to move into.
There was a problem hiding this comment.
It's not our only option. One of the challenges with big monolithic tables is that they have to fit in a single hardware stage. If we break up tables and allow them to be distributed across stages using approaches like algorithmic TCAM then we can potentially achieve a higher upper bound on total usable table space.
I think a table size of 1024 is a reasonable starting point, as we know this number will need to grow significantly to meet customer requirements.
Commit on main: c44dd2504f2a623e640d94955376cdb5cadd7ed6
Commit on main: c44dd2504f2a623e640d94955376cdb5cadd7ed6
This brings multicast group management and hardware-accelerated forwarding to Dendrite, providing the foundation for our multicast networking stack.
The PR includes:
API Layer:
Hardware Integration:
Network Processing:
Validation:
Context:
Associated PRs